Refactor PostgreSQLExecutor to support Windows compatibility for process management#1182
Open
tboy1337 wants to merge 35 commits intodbfixtures:mainfrom
Open
Refactor PostgreSQLExecutor to support Windows compatibility for process management#1182tboy1337 wants to merge 35 commits intodbfixtures:mainfrom
tboy1337 wants to merge 35 commits intodbfixtures:mainfrom
Conversation
Open
fizyk
reviewed
Sep 4, 2025
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (8)
✅ Files skipped from review due to trivial changes (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe PR introduces Windows compatibility for PostgreSQL executor management by implementing platform-specific process start command templates (Unix and Windows variants), adding Windows-specific process termination logic with timeout handling, updating subprocess invocation patterns, and extending CI to test Windows environments with three PostgreSQL versions. Changes
Sequence DiagramssequenceDiagram
participant App as Application
participant Exec as PostgreSQLExecutor
participant Proc as subprocess.Popen
participant PgCtl as pg_ctl
App->>Exec: stop(sig=None)
Exec->>PgCtl: subprocess.run([executable, "stop", "-D", datadir, "-m", "f"])
PgCtl-->>Exec: returncode
alt Windows Platform
Exec->>Exec: _windows_terminate_process(None)
Exec->>Proc: terminate()
rect rgba(255, 100, 100, 0.5)
Exec->>Proc: wait(timeout=30)
Note over Proc: May raise TimeoutExpired
end
alt Timeout Occurred
Exec->>Proc: kill()
Exec->>Proc: wait()
end
else Unix Platform
Exec->>Exec: TCPExecutor.stop(sig, exp_sig)
Note over Exec: Uses os.killpg
end
Exec-->>App: Return
sequenceDiagram
participant App as Application
participant Exec as PostgreSQLExecutor
participant SubProc as subprocess
App->>Exec: running()
Exec->>SubProc: run([executable, "status", "-D", datadir], check=False, shell=False)
SubProc-->>Exec: CompletedProcess(returncode=...)
alt returncode == 0
Exec-->>App: True (running)
else
Exec-->>App: False (not running)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@pytest_postgresql/executor.py`:
- Around line 263-283: In stop(self, sig: Optional[int] = None, exp_sig:
Optional[int] = None) replace the subprocess.check_output call that builds a
shell string with a list-style invocation (e.g., [self.executable, "stop", "-D",
self.datadir, "-m", "f"]) and remove shell=True so subprocess runs the
executable directly; this avoids breaking paths with spaces and removes the
shell injection risk while keeping the same arguments and behavior before the
platform-specific termination logic (_windows_terminate_process / super().stop).
- Around line 53-66: The _get_base_command constructs the -o string with
unix_socket_directories unquoted which breaks for paths with spaces; update
_get_base_command to detect whitespace in the unixsocketdir variable and wrap
that value in double quotes (or escape quotes) when building the -o option so
the produced fragment becomes -c unix_socket_directories="/path/with space"
(referencing the unix_socket_directories token and BASE_PROC_START_COMMAND
assignment), and add a unit test that passes a datadir/unixsocketdir containing
a space to ensure the command line includes the quoted socket directory (or
alternatively, add explicit validation/documentation and raise a clear error
when whitespace is present).
🧹 Nitpick comments (1)
pytest_postgresql/executor.py (1)
231-262: Tidy up the termination helper API surface.
sigis unused in_windows_terminate_process, and_unix_terminate_processis currently unused. Either wire_unix_terminate_processintostop()for symmetry and usesig, or drop/rename the parameter to avoid dead code and lint noise.
tboy1337
force-pushed
the
edit
branch
2 times, most recently
from
5d4fb91 to
efa4633
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@tests/test_windows_compatibility.py`:
- Around line 35-39: The test assertions in tests/test_windows_compatibility.py
don't match the actual string produced by _get_base_command(): update the
assertions to expect the escaped-quote form for unix_socket_directories (i.e.,
include the backslash-escaped double quotes around {unixsocketdir}) and assert
that the non-quoted form is not present; specifically change the positive
assertion referencing unix_socket_directories to look for
unix_socket_directories=\"{unixsocketdir}\" and keep the negative assertion for
unix_socket_directories='{unixsocketdir}' to ensure consistency with
_get_base_command().
- Around line 189-204: The test's expected_parts entry for
unix_socket_directories doesn't match the actual command string (which includes
escaped quotes), so update the expectation in the test to match the
implementation: replace the "-c unix_socket_directories=C:/temp/socket" entry
with the escaped-quote form that appears in executor.command (e.g., -c
unix_socket_directories=\"C:/temp/socket\"), keeping the rest of the
expected_parts and the loop over executor.command unchanged; reference the test
variable executor.command and the expected_parts list when making the change.
🧹 Nitpick comments (2)
pytest_postgresql/executor.py (2)
25-25: Unused importsignal.The
signalmodule is imported but not used anywhere in the file. This appears to be a leftover from development.🧹 Suggested fix
-import signal
250-269: Good refactor to list-based subprocess call.The change from shell string to list format (
[self.executable, "stop", "-D", self.datadir, "-m", "f"]) properly addresses the previous security concern and handles paths with spaces correctly.One minor observation: the
AttributeErrorfallback (lines 263-268) uses string matching"killpg" in str(e)which is somewhat fragile. Consider whether this fallback is still needed given the explicitplatform.system()check now handles Windows upfront.
tboy1337
force-pushed
the
edit
branch
2 times, most recently
from
b02c61b to
4a0b22e
Compare
fizyk
reviewed
Jan 23, 2026
tboy1337
force-pushed
the
edit
branch
2 times, most recently
from
059e4bc to
54140ac
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@pytest_postgresql/executor.py`:
- Around line 226-246: The parameter sig in _windows_terminate_process is
intentionally unused and triggers Ruff ARG002; either rename it to _sig to
indicate it’s intentionally unused or keep sig and add a local lint suppression
(e.g., append a noqa comment for ARG002) where the function is defined to
silence the warning; update the function signature (or add the noqa) in the
_windows_terminate_process definition and run linters to confirm the ARG002
warning is resolved.
fizyk
approved these changes
Jan 29, 2026
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/single-postgres.yml (1)
82-86:⚠️ Potential issue | 🟡 MinorArtifact upload path is Unix-only.
/tmp/pytest-of-runner/**on line 86 won't match anything on Windows. This is gated behindif: failure()so it won't fail the build, but Windows test failure artefacts will be silently lost. Consider adding a platform-conditional path or using${{ runner.temp }}.
🤖 Fix all issues with AI agents
In @.github/workflows/single-postgres.yml:
- Around line 59-62: The workflow hardcodes POSTGRESQL_EXEC to
/usr/lib/postgresql/... which breaks macOS runners; change the step that sets
POSTGRESQL_EXEC (the block under the if: runner.os != 'Windows') to detect
pg_ctl on Unix-like systems instead of a Debian path — use command -v pg_ctl and
if that fails try macOS Homebrew locations (e.g.,
/opt/homebrew/opt/postgresql@${{ inputs.postgresql }}/bin/pg_ctl and
/usr/local/opt/postgresql@${{ inputs.postgresql }}/bin/pg_ctl) or set a fallback
to the current Debian path; ensure the final value exported as POSTGRESQL_EXEC
is the first existing pg_ctl found so CI on macos_postgres_* with
ankane/setup-postgres works correctly.
🧹 Nitpick comments (4)
pytest_postgresql/executor.py (2)
226-246: Broadly swallowing exceptions in cleanup is acceptable but consider logging.The
except (OSError, AttributeError): passon lines 244–246 silently discards allOSErrorvariants (including permission errors) andAttributeError. For a teardown helper this is pragmatic, but a debug-level log would aid troubleshooting when processes fail to terminate.
261-266: String-basedAttributeErrormatching is fragile.Checking
"killpg" in str(e)couples the fallback to the exact wording of CPython's error message, which could change across versions or implementations. Consider catching the missing attribute directly.Proposed fix
except AttributeError as e: - # Fallback for edge cases where os.killpg doesn't exist - if "killpg" in str(e): + # Fallback for edge cases where os.killpg doesn't exist (e.g. Windows) + if not hasattr(os, "killpg"): self._windows_terminate_process(sig) else: raisetests/conftest.py (1)
7-9: Remove the commented-out import rather than leaving dead code.The commented-out line still triggers a Ruff warning (RUF100) for the stale
noqadirective. Since the comment on line 8 already explains the rationale, the old import line adds no value.Proposed fix
# Plugin is registered via entry point in pyproject.toml - no need to import here -# from pytest_postgresql.plugin import * # noqa: F403,F401tests/test_windows_compatibility.py (1)
74-98: Patch target forsubprocess.check_outputshould be where it's looked up.The patch
patch("subprocess.check_output")works becauseexecutor.pyusesimport subprocessand thensubprocess.check_output(...), so patching the module attribute is fine. However, best practice is to patch where the name is used:pytest_postgresql.executor.subprocess.check_output. This avoids accidentally affecting other code if tests are ever run in parallel or the import style changes.This applies to all test methods in this class that patch
subprocess.check_output.
Contributor
Author
|
@coderabbitai resume |
Contributor
✅ Actions performedReviews resumed. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@tests/test_windows_compatibility.py`:
- Around line 127-155: The test fails on Unix because os.killpg exists so the
stop() except branch re-raises; to exercise the fallback, remove killpg from the
executor module's os before calling stop: inside the test's with-block, import
sys and call delattr(sys.modules['pytest_postgresql.executor'].os, "killpg") (or
use monkeypatch.delattr on that module's os) so that hasattr(os, "killpg") is
False when PostgreSQLExecutor.stop() runs; keep references to
PostgreSQLExecutor.stop, os.killpg, and _windows_terminate_process to locate the
code to change.
🧹 Nitpick comments (3)
pytest_postgresql/executor.py (2)
32-34: Logger initialisation breaks import grouping.The
logger = logging.getLogger(__name__)statement is sandwiched between twomirakuruimports. Move it after all top-level imports to keep the import block contiguous and PEP 8–compliant.Suggested reordering
from mirakuru import TCPExecutor - -logger = logging.getLogger(__name__) from mirakuru.exceptions import ProcessFinishedWithError from packaging.version import parse from pytest_postgresql.exceptions import ExecutableMissingException, PostgreSQLUnsupported + +logger = logging.getLogger(__name__)
229-254:_windows_terminate_processlooks good overall; minor nit on redundantpass.The graceful-then-force-kill approach is sound. However, the
passon line 254 is redundant — thelogger.debug(...)call already satisfies the except block body.Suggested fix
logger.debug( "Exception during Windows process termination: %s: %s", type(e).__name__, e, ) - passtests/test_windows_compatibility.py (1)
74-98: Windows stop test: consider asserting the exact arguments passed tocheck_output.The test asserts
mock_subprocess.assert_called_once()but doesn't verify the arguments. Sincestop()now uses list-form args ([self.executable, "stop", "-D", self.datadir, "-m", "f"]), asserting the call args would catch regressions in the command construction.Optional enhancement
# Should call pg_ctl stop and Windows terminate - mock_subprocess.assert_called_once() + mock_subprocess.assert_called_once_with( + ["/path/to/pg_ctl", "stop", "-D", "/tmp/data", "-m", "f"], + ) mock_terminate.assert_called_once()
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@pytest_postgresql/executor.py`:
- Around line 244-246: Replace the blocking self.process.wait() after
self.process.kill() with a timed wait and a fallback path: call
self.process.wait(timeout=...) (or poll in a short loop) and catch
subprocess.TimeoutExpired, then log a warning and avoid hanging (optionally
attempt a forceful termination or continue without blocking); update the code
around self.process.kill() / self.process.wait() in executor.py to use a timeout
and warning on failure to reap the process.
🧹 Nitpick comments (1)
tests/test_windows_compatibility.py (1)
74-100: Consider a more precise mock assertion for_windows_terminate_process.
mock_terminate.assert_called_once()on line 99 does not verify the arguments. Sincestop()callsself._windows_terminate_process(sig)(wheresigdefaults toNone), usingassert_called_once_with(None)would make the test stricter and catch regressions if the call signature changes.Proposed fix
- mock_terminate.assert_called_once() + mock_terminate.assert_called_once_with(None)
fizyk
approved these changes
Feb 11, 2026
fizyk
reviewed
Feb 12, 2026
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @.github/workflows/single-postgres.yml:
- Around line 50-62: The Windows step "Detect PostgreSQL path on Windows"
currently may leave POSTGRESQL_EXEC unset if neither the hardcoded Test-Path
lookup nor Get-Command finds pg_ctl; update the pwsh run block so that after
attempting both Test-Path and Get-Command you check whether $pgPath is set and,
if not, write an explicit error and exit non‑zero (e.g., write-host/error then
exit 1) so downstream steps that rely on POSTGRESQL_EXEC (the env var) fail fast
and consistently rather than receiving an empty --postgresql-exec.
Contributor
Author
|
@coderabbitai resume |
- Added steps to install the package in editable mode before running tests in dockerised-postgres.yml, oldest-postgres.yml, and single-postgres.yml. - Streamlined the testing process by ensuring the package is available in the current environment for all workflows.
- Modified the package installation command to include the --no-deps flag, ensuring that only the editable package is installed without its dependencies before running tests.
- Added verification step to ensure pg_ctl is found in expected locations during the GitHub Actions workflow for single-postgres.yml. - Implemented error logging and exit strategy if PostgreSQL is not detected, improving robustness of the setup process.
- Introduced separate command templates for Unix and Windows to handle quoting and configuration differences. - Updated tests to validate the correct command template is used based on the operating system. - Enhanced test coverage for command formatting, ensuring proper handling of paths with spaces on Unix and omission of unnecessary parameters on Windows.
- Updated the pg_ctl path in single-postgres.yml to include the .exe extension for compatibility with Windows environments, ensuring proper execution of PostgreSQL commands.
- Changed the conftest file generation to use a more explicit pytest_plugins declaration for better clarity and compatibility with pytest's plugin system.
…for improved clarity - Changed the pytest_plugins declaration in the conftest file generation to specify the plugin path explicitly, enhancing compatibility with pytest's plugin system.
…tgreSQLExecutor - Added tests to verify correct command templates for Windows, Unix, and Darwin platforms, ensuring proper handling of `unix_socket_directories` and `log_destination`. - Implemented checks for locale settings on Darwin and preserved quoting for `postgres_options` across different platforms. - Improved test assertions for command generation with special characters and empty parameters, enhancing overall test robustness.
…andling - Introduced a pytest fixture to set Windows-compatible locale environment variables, ensuring compatibility with PostgreSQL's initdb on Windows. - Updated test cases for PostgreSQL start commands across Windows, Unix, and Darwin platforms to include a password parameter, enhancing test coverage and consistency.
… fixture from conftest.py - Modified locale settings in executor.py to ensure compatibility across Darwin and Windows platforms. - Removed the Windows locale setup fixture from conftest.py as the locale handling is now managed directly in executor.py, simplifying the test configuration.
…Executor - Updated the test case to create a dedicated socket directory for Unix systems, improving clarity and organization in the test setup. - Adjusted the path for the `unixsocketdir` parameter to use the newly created socket directory, ensuring proper configuration for PostgreSQLExecutor.
- Modified the upload path for pytest results to include both the temporary runner directory and a specific /tmp directory, ensuring comprehensive coverage of test artifacts during the workflow execution.
…etemp for improved temporary directory handling - Added the --basetemp option to pytest commands to specify a base temporary directory, enhancing the management of test artifacts during execution. - Simplified the upload path for pytest results to focus on the temporary runner directory, ensuring efficient artifact collection.
…t collection - Simplified the upload path for pytest results by removing the specific wildcard in the temporary directory, ensuring more efficient collection of test artifacts during the workflow execution.
Upgraded fizyk/actions-reuse pipenv-setup action from v4.2.1 to v4.4.0 which includes built-in support for editable package installation via the editable flag. This simplifies the workflows by removing the explicit pip install -e . steps. Changes: - Updated pipenv-setup to v4.4.0 in all workflow files - Added editable: true parameter to pipenv-setup steps - Removed separate "Install package in editable mode" steps from dockerised-postgres.yml, single-postgres.yml, and oldest-postgres.yml This aligns with the upstream pattern established in pytest-mongo and reduces workflow complexity while maintaining the same functionality. Co-authored-by: Cursor <[email protected]>
… checks - Upgraded the pipenv-run action to v4.2.1 in dockerised-postgres.yml, single-postgres.yml, and oldest-postgres.yml for consistency across workflows. - Refined the conditional check for installing libpq to use matrix.python-version instead of inputs.python-versions in oldest-postgres.yml and single-postgres.yml, improving clarity and accuracy. These changes enhance the maintainability and consistency of the workflow configurations.
…est_postgres_options_plugin.py - Updated the socket directory creation in test_executor.py to use basetemp for improved path management, addressing Unix domain socket length limitations. - Removed unnecessary conftest configuration in test_postgres_options_plugin.py to streamline the pytest setup. These changes enhance the clarity and efficiency of the test configurations.
… assertions - Removed unnecessary spaces in the command templates for both Unix and Windows to ensure proper execution of PostgreSQL commands. - Updated test assertions in test_executor.py and test_windows_compatibility.py to check for the correct formatting of the `unix_socket_directories` parameter, ensuring consistency across platforms. These changes enhance the reliability of command execution and improve test accuracy.
…utor.py - Adjusted assertions in test_actual_postgresql_start_unix and test_actual_postgresql_start_darwin to check for correct formatting of the `unix_socket_directories` parameter, ensuring consistency in command output. These changes improve the accuracy of the tests related to PostgreSQL command execution.
- Updated the platform_name parameter in test_executor.py to include "FreeBSD", expanding the test coverage for platform-specific functionality. This change enhances the robustness of the tests by ensuring compatibility with FreeBSD.
- Updated the --basetemp option in pytest commands to specify a consistent temporary directory for test artifacts, improving organization and clarity in the workflow. - Adjusted the artifact upload path to reflect the new temporary directory structure, ensuring efficient collection of test results. These changes enhance the maintainability and reliability of the testing workflow.
for more information, see https://pre-commit.ci
…tibility.py - Changed the docstring to use a raw string literal for better handling of backslashes in UNC path examples. - Clarified the description of UNC paths to ensure accurate representation of their format. These updates improve the clarity and correctness of the test documentation.
The [email protected] upgrade and editable: true flag additions have been extracted into a dedicated PR (dbfixtures#1294). Remove those changes from this branch to avoid duplication and conflicts once that PR is merged. Made-with: Cursor
- Updated the command templates in `PostgreSQLExecutor` to ensure the executable path is always double-quoted, improving compatibility with paths containing spaces. - Modified tests in `test_windows_compatibility.py` to reflect the changes in command formatting, ensuring that single-quoted PostgreSQL options are preserved correctly within double-quoted command strings. - Added new tests to verify that the `running()` method properly quotes the executable and datadir, enhancing robustness against shell parsing issues.
for more information, see https://pre-commit.ci
- Updated the command generation in `PostgreSQLExecutor` to escape single quotes in the `unixsocketdir` parameter, ensuring valid syntax for PostgreSQL GUC strings. - Refactored the `running()` method to use `subprocess.run` instead of `subprocess.getstatusoutput`, improving compatibility and security by avoiding shell parsing. - Added regression tests in `test_windows_compatibility.py` to verify correct escaping of apostrophes in `unixsocketdir` and ensure robust command generation.
for more information, see https://pre-commit.ci
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Refactors
PostgreSQLExecutorto improve Windows compatibility and security for PostgreSQL process management. This change addresses platform-specific issues with process termination and command formatting that could cause failures on Windows systems.Changes
_get_base_commandmethod toBASE_PROC_START_COMMANDclass attribute with cross-platform compatible syntax (removed single quotes around PostgreSQL config values that caused issues on Windows)_windows_terminate_processmethod that attempts graceful shutdown before force-killing stuck processesstop()method to use list arguments instead ofshell=Trueto prevent shell injection vulnerabilitiesos.killpgAttributeError on WindowsTest plan
tests/test_windows_compatibility.pywith 7 test caseskillpgAttributeErrorSummary by CodeRabbit
Release Notes
New Features
Tests